Skip to content

Fix #201: focussearch now handles discrete vector parameters (cont.) #257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix #201: focussearch now handles discrete vector parameters (cont.) #257

wants to merge 5 commits into from

Conversation

@jakob-r
Copy link
Member

@jakob-r jakob-r commented Sep 19, 2016

I was not able to take over PR #202 and keep it there. But I was able to pull the branch here it is

mb706 and others added 5 commits March 6, 2016 00:22
Fix #201: focussearch now handles discrete vector parameters
5b32e6e 
also general focussearch bugfixes
More concise handling of discrete vector params in infillOptFocus
d451c63 
Parameters that have discrete vector params in their requirement can
not be handled by infillOptFocus, so trying to do that now gives an
error.
Cleaning up code.
ecdeca8
@jakob-r
Merge branch 'master' into fix201_focussearch
6befed2
@jakob-r
naming according to styleguide
15b7e6c
@jakob-r jakob-r mentioned this pull request Sep 19, 2016
Closed
@jakobbossek jakobbossek added this to the v0.1 milestone Oct 26, 2016
@jakobbossek jakobbossek mentioned this pull request Oct 27, 2016
Open
mllg
mllg reviewed Nov 18, 2016
View reviewed changes
R/infillOptFocus.R
to.del = sample(val.names, 1)
newmap = newmap[newmap != to.del]
names(newmap) = names(par$values)
discrete.vector.mapping[[dfindex]] <<- newmap
Copy link
Member

@mllg mllg Nov 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<<-?

Copy link
Contributor

@ja-thomas ja-thomas Nov 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need the updated discrete.vector.mapping outside of the lapply

mllg
mllg reviewed Nov 18, 2016
View reviewed changes
R/infillOptFocus.R
if (par$type %nin% c("discretevector", "logicalvector")) {
val.names = names(par$values)
# remove current val from delete options, should work also for NA
val.names = val.names[!sapply(par$values, identical, y=val)] # remember, 'val' can be any type
Copy link
Member

@mllg mllg Nov 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vlapply

mllg
mllg reviewed Nov 18, 2016
View reviewed changes
R/infillOptFocus.R
}
} else if (isDiscrete(par)) {
# randomly drop a level, which is not val
if (length(par$values) <= 1L) {
Copy link
Member

@mllg mllg Nov 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== 0

mllg
mllg reviewed Nov 18, 2016
View reviewed changes
tests/testthat/test_infill_opt_focus.R
if(x$disc2 == "c") tmp3 = 500
assert(is.list(x$discVec))
assert(x$discVec[[1]] %in% c("a", "b", "c"))
assert(x$discScal %in% c("x", "y", "z"))
Copy link
Member

@mllg mllg Nov 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not what assert is for. Use expect_is/expect_true and expect_subset

@berndbischl
Copy link
Member

berndbischl commented Nov 18, 2016

i will take this now.

@ja-thomas
Copy link
Contributor

ja-thomas commented Nov 24, 2016

Just had a look,

a) general look/review
b) rebased to master
c) fixed most of the stuff michel mentioned

afaik good2go now (if travis is green)

@ja-thomas
Copy link
Contributor

ja-thomas commented Dec 19, 2016

ping @berndbischl

@berndbischl
Copy link
Member

berndbischl commented Dec 23, 2016

this is really not so good what happens here:

  1. i cannot reproduce the error with the suggested new unit test. if i run that test, without further code changes, no error is reproduced. so the unit test is not "catching"

  2. the suggested fix looks suboptimal. code becomes cluttered and unelegant. my suggestion is:
    use the new splitVectorParamsFunction from PH, convert the parset in the focus-search to only "single" params and work on those. that should be much simpler.

@jakob-r
Copy link
Member Author

jakob-r commented Jan 5, 2017 •
edited
Loading

Okay. So we are waiting for the PR in ParamHelpers for "splitVectorParamsFunction" and then we will refactor the code.

@jakob-r jakob-r mentioned this pull request May 12, 2017
Open
@jakob-r jakob-r force-pushed the fix201_focussearch branch from 9648acf to 15b7e6c Compare May 22, 2017 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mllg mllg mllg left review comments

+1 more reviewer

@ja-thomas ja-thomas ja-thomas left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@berndbischl berndbischl

Labels

prio-high ready for merge (?)

Projects

None yet

Milestone

v0.1

Development

Successfully merging this pull request may close these issues.

6 participants

@jakob-r @berndbischl @ja-thomas @mllg @jakobbossek